Create separate automodel pack by starcke · Pull Request #13879 · github/codeql

starcke · 2023-08-03T12:01:30Z

This is a first attempt at making a separate automodel pack. This was done together with @kaeluka.

We were not super sure if this was the exact right thing to do, but it seems a starting point.

In VS code we are seeing a bunch of red lines:

However if we change the dependency to

codeql/java-all: '*'

then we still have the red lines but the queries are able to run just fine.

aeisenberg

I think you need to add this pack to the codeql-workspace.yml file in the root of the repo. I'm not sure if that's all that's going on. I can try this out later today.

starcke · 2023-08-03T13:56:22Z

I think you need to add this pack to the codeql-workspace.yml file in the root of the repo. I'm not sure if that's all that's going on. I can try this out later today.

Great! that fixed VS Code for me 🎉

kaeluka · 2023-08-03T14:11:47Z

Can confirm: no squiggles over here either! Thank you, @aeisenberg <3

aeisenberg · 2023-08-03T15:19:15Z

Fantastic. 😄 Are you seeing any other problems?

aeisenberg · 2023-08-03T15:22:22Z

You probably want to add a groups property to this file. Maybe this:

groups: - java - automodel

This property controls how and when to run pack commands on slices of workspace packs. This way, when you add automodel packs for other languages, you can publish all of them by using the command codeql pack publish --groups automodel,-test (This will publish all non-test automodel packs.)

So, you will want to add a similar group to the test pack (and add test as one of its groups.

Check out the other packs for inspiration.

Oh...I see. The tests are included in the same pack. They should probably be split into its own pack. This way you can publish the queries without including the tests.

You probably want to add a groups property to this file.

I saw that but couldnt find what it actually did in the documentation. I'll add it.

Oh...I see. The tests are included in the same pack.

Yeah we discussed this, my feeling was that having two separate packs was a bit too much process for such a small pack. But if the standard is to always split then I'll make that change as well.

aeisenberg · 2023-08-04T17:21:46Z

+echo Bumping versions
+"${CODEQL_DIST}/codeql" pack post-release --groups $GRPS
+
+echo Automodel packs successfully published. Please commit and push the version changes.


If you wanted to be extra fancy, you could commit and push directly from this script.

Hmm... I kind of like it not committing for me, then there is less that suddenly happens. But we can also tweak this afterwards. It almost seems like there should be a generic publish script we could have for all the separate packs, but I am not sure how frequent these separate packs are?

If this script is going to be run in CI, then something will have to commit and push the change, but maybe that is a longer term goal.

Eventually, all of our core packs will be publishable separately. But, that won't happen for a while. I still would hope that something like this script would become the default way of publishing.

aeisenberg

Trying this locally. Looks like the qlref files are no longer correct. You need to remove the Telemetry/ path segment.

Also, looks like I need a newer codeql version installed. I'm in a place with slow internet, so downloading and will try again when it's ready.

aeisenberg · 2023-08-04T17:25:44Z

+WORKSPACE_ROOT="$AUTOMODEL_ROOT/../../../.."
+GRPS="automodel,-test"
+
+if [ -z "$CODEQL_DIST" ]; then


(Optional)...you could also check if gh codeql is available and use that if CODEQL_DIST is unset.

Lets defer that in case we want to make changes to the solorigate pack as well.

aeisenberg · 2023-08-04T17:57:10Z

+set -e
+
+AUTOMODEL_ROOT="$(dirname $0)"
+WORKSPACE_ROOT="$AUTOMODEL_ROOT/../../../.."


Suggested change

WORKSPACE_ROOT="$AUTOMODEL_ROOT/../../../.."

WORKSPACE_ROOT="$(realpath "$AUTOMODEL_ROOT/../../..")"

I have always been using readlink -f do you know if realpath is ok on all the places this could be thought to run?

I'm not sure. I've usually just used realpath in situations like this. If readlnk works, too then that's probably fine.

aeisenberg · 2023-08-04T18:01:45Z

Upgraded to v2.14.1 and most tests are passing now. The AutomodelFrameworkModeExtractCandidates test is still failing.

Publishing and pre/post release process is working. I published and deleted the pack right after. Don't forget that after you publish, you need to manually change the visibility of that pack from internal to public.

aeisenberg · 2023-08-04T18:05:53Z

+#!/bin/sh
+set -e
+
+AUTOMODEL_ROOT="$(dirname $0)"


I know that the original script has this same line (which I wrote a couple of years ago), but not sure if it's the best. This script is only running successfully from the codeql/java/ql/automodel directory. Running in the repo root is failing for some reason.

Ah...I think the path in line 5 needs to be normalized. Running from the repo root will make WORKSPACE_ROOT=java/ql/automodel/../../.., but it really needs to be WORKSPACE_ROOT=..

Suggested change

AUTOMODEL_ROOT="$(dirname $0)"

AUTOMODEL_ROOT="$(realpath "$(dirname $0)")"

I have changed this to do:

AUTOMODEL_ROOT="$(readlink -f "$(dirname $0)")" WORKSPACE_ROOT="$AUTOMODEL_ROOT/../../../.."

Happy to switch to realpath as well if that is better.

Also updated the solorigate script with this change.

starcke · 2023-08-09T07:17:00Z

Merged in master after merging #13852. Moved the MaD file to the new directory and updated it.

kaeluka · 2023-08-09T07:49:33Z

Do you think this is ready to be merged, @aeisenberg?

aeisenberg

I think this looks good. We can make more changes in the future if we are going to fully automate the script. Nice job!

aeisenberg · 2023-08-14T20:18:59Z

+set -e
+
+AUTOMODEL_ROOT="$(dirname $0)"
+WORKSPACE_ROOT="$AUTOMODEL_ROOT/../../../.."


I'm not sure. I've usually just used realpath in situations like this. If readlnk works, too then that's probably fine.

aeisenberg · 2023-08-14T20:22:54Z

+echo Bumping versions
+"${CODEQL_DIST}/codeql" pack post-release --groups $GRPS
+
+echo Automodel packs successfully published. Please commit and push the version changes.


If this script is going to be run in CI, then something will have to commit and push the change, but maybe that is a longer term goal.

Eventually, all of our core packs will be publishable separately. But, that won't happen for a while. I still would hope that something like this script would become the default way of publishing.

starcke · 2023-08-17T08:10:33Z

There were some conflicts with #13886 but command-line git resolved them directly.

starcke · 2023-08-17T10:23:30Z

When trying to publish the automodel pack I ran into a single test failure:

Executing 6 tests in 2 directories.                                                                   
Extracting test database in /home/starcke/github/vcs/codeql/java/ql/automodel/test/AutomodelFrameworkModeExtraction.
Compiling queries in /home/starcke/github/vcs/codeql/java/ql/automodel/test/AutomodelFrameworkModeExtraction.
Compiled /home/starcke/github/vcs/codeql/java/ql/automodel/test/AutomodelFrameworkModeExtraction/AutomodelFrameworkModeExtractCandidates.qlref (908ms).
Compiled /home/starcke/github/vcs/codeql/java/ql/automodel/test/AutomodelFrameworkModeExtraction/AutomodelFrameworkModeExtractPositiveExamples.qlref (280ms).
Compiled /home/starcke/github/vcs/codeql/java/ql/automodel/test/AutomodelFrameworkModeExtraction/AutomodelFrameworkModeExtractNegativeExamples.qlref (200ms).
Executing tests in /home/starcke/github/vcs/codeql/java/ql/automodel/test/AutomodelFrameworkModeExtraction.
--- expected                                                                                          
+++ actual                                                                                            
@@ -1,7 +1,1 @@                                                                                       
-| com/github/codeql/test/PublicClass.java:4:15:4:19 | stuff | command-injection, path-injection, request-forgery, sql-injection\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@. | com/gi
thub/codeql/test/PublicClass.java:4:15:4:19 | stuff | MethodDoc | com/github/codeql/test/PublicClass.java:4:15:4:19 | stuff | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | pa
ckage | file://PublicClass:1:1:1:1 | PublicClass | type | file://true:1:1:1:1 | true | subtypes | file://stuff:1:1:1:1 | stuff | name | file://(String):1:1:1:1 | (String) | signature | file://Argument[thi
s]:1:1:1:1 | Argument[this] | input | file://this:1:1:1:1 | this | parameterName |                    
-| com/github/codeql/test/PublicClass.java:4:21:4:30 | arg | command-injection, path-injection, request-forgery, sql-injection\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@. | com/gith
ub/codeql/test/PublicClass.java:4:21:4:30 | arg | MethodDoc | com/github/codeql/test/PublicClass.java:4:21:4:30 | arg | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package 
| file://PublicClass:1:1:1:1 | PublicClass | type | file://true:1:1:1:1 | true | subtypes | file://stuff:1:1:1:1 | stuff | name | file://(String):1:1:1:1 | (String) | signature | file://Argument[0]:1:1:1:
1 | Argument[0] | input | file://arg:1:1:1:1 | arg | parameterName |                                                                                                                                        
-| com/github/codeql/test/PublicClass.java:8:34:8:43 | arg | command-injection, path-injection, request-forgery, sql-injection\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@. | com/gith
ub/codeql/test/PublicClass.java:8:34:8:43 | arg | MethodDoc | com/github/codeql/test/PublicClass.java:8:34:8:43 | arg | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package 
| file://PublicClass:1:1:1:1 | PublicClass | type | file://false:1:1:1:1 | false | subtypes | file://staticStuff:1:1:1:1 | staticStuff | name | file://(String):1:1:1:1 | (String) | signature | file://Argu
ment[0]:1:1:1:1 | Argument[0] | input | file://arg:1:1:1:1 | arg | parameterName |
-| com/github/codeql/test/PublicInterface.java:4:17:4:21 | stuff | command-injection, path-injection, request-forgery, sql-injection\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@. | co
m/github/codeql/test/PublicInterface.java:4:17:4:21 | stuff | MethodDoc | com/github/codeql/test/PublicInterface.java:4:17:4:21 | stuff | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.code
ql.test | package | file://PublicInterface:1:1:1:1 | PublicInterface | type | file://true:1:1:1:1 | true | subtypes | file://stuff:1:1:1:1 | stuff | name | file://(String):1:1:1:1 | (String) | signature |
 file://Argument[this]:1:1:1:1 | Argument[this] | input | file://this:1:1:1:1 | this | parameterName | 
-| com/github/codeql/test/PublicInterface.java:4:23:4:32 | arg | command-injection, path-injection, request-forgery, sql-injection\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@. | com/
github/codeql/test/PublicInterface.java:4:23:4:32 | arg | MethodDoc | com/github/codeql/test/PublicInterface.java:4:23:4:32 | arg | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.tes
t | package | file://PublicInterface:1:1:1:1 | PublicInterface | type | file://true:1:1:1:1 | true | subtypes | file://stuff:1:1:1:1 | stuff | name | file://(String):1:1:1:1 | (String) | signature | file:
//Argument[0]:1:1:1:1 | Argument[0] | input | file://arg:1:1:1:1 | arg | parameterName |
-| com/github/codeql/test/PublicInterface.java:6:36:6:45 | arg | command-injection, path-injection, request-forgery, sql-injection\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@. | com/
github/codeql/test/PublicInterface.java:6:36:6:45 | arg | MethodDoc | com/github/codeql/test/PublicInterface.java:6:36:6:45 | arg | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.tes
t | package | file://PublicInterface:1:1:1:1 | PublicInterface | type | file://false:1:1:1:1 | false | subtypes | file://staticStuff:1:1:1:1 | staticStuff | name | file://(String):1:1:1:1 | (String) | sig
nature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://arg:1:1:1:1 | arg | parameterName |                                                                                                      
-| java/nio/file/Files.java:10:9:10:24 | out | command-injection, path-injection, request-forgery, sql-injection\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@. | java/nio/file/Files.ja
va:10:9:10:24 | out | MethodDoc | java/nio/file/Files.java:10:9:10:24 | out | ClassDoc | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1
 | false | subtypes | file://copy:1:1:1:1 | copy | name | file://(Path,OutputStream):1:1:1:1 | (Path,OutputStream) | signature | file://Argument[1]:1:1:1:1 | Argument[1] | input | file://out:1:1:1:1 | out
 | parameterName |                                                                                    
+                                                                                                     
[1/6 comp 908ms eval 1.8s] FAILED(RESULT) /home/starcke/github/vcs/codeql/java/ql/automodel/test/AutomodelFrameworkModeExtraction/AutomodelFrameworkModeExtractCandidates.qlref
Location is outside of test directory: file://java.nio.file:1:1:1:1
Location is outside of test directory: file://Files:1:1:1:1     
Location is outside of test directory: file://false:1:1:1:1                
Location is outside of test directory: file://copy:1:1:1:1
Location is outside of test directory: file://(Path,OutputStream):1:1:1:1                                                                                                                                   
Location is outside of test directory: file://Argument[0]:1:1:1:1  
Location is outside of test directory: file://source:1:1:1:1
Locations outside the test directory do not work well for regression tests.

@kaeluka does that also happen for you, and do you have any thoughts on why that would happen? I tried to reproduce it on main but the test seem to pass there, and there doesnt seem to be any real differences between the queries or the tests.

kaeluka · 2023-08-17T12:03:20Z

These warnings Location is outside have been around since forever. They have to do with how we abuse the file location info to export data into the sarif format. May still be connected to the problem, though — who knows. Look up the DollarAtString class for details.
The behaviour is freaky: when I run the tests locally, 5/6 pass, only candidate extraction in framework mode fails (it finds no candidates at all).
This test fails locally, but the CI checks are green — does that mean that our new test suite is no longer executed by the CI checks? Or does it behave differently when run in the CI checks?

kaeluka · 2023-08-17T12:28:26Z

I'll take a look tomorrow to see if I can find any explanation for why only one query is broken.

…ocation

kaeluka · 2023-08-18T08:33:57Z

I just fixed the issue — see my commit 480e3bf.

Tests pass locally 😄

starcke · 2023-08-18T10:08:06Z

I just fixed the issue — see my commit 480e3bf.

Woa, I would never have guessed that - thanks!

aeisenberg

This is good enough for now. We need to determine if we are going to do anything about the -dev dependencies. This can happen later.

starcke · 2023-08-30T07:28:19Z

I had to manually merge in master due to some changed files, but command-line git could do it automatically without having to resolve conflicts. The extension has also been updated to use this pack. So after this gets another stamp it should finally be good to merge.

Create separate automodel pack.

e2abd3f

github-actions Bot added the Java label Aug 3, 2023

aeisenberg reviewed Aug 3, 2023

View reviewed changes

Update workspace.

e7ca233

aeisenberg reviewed Aug 3, 2023

View reviewed changes

Address comments.

3ef82c1

starcke marked this pull request as ready for review August 4, 2023 11:08

starcke requested a review from a team as a code owner August 4, 2023 11:08

aeisenberg reviewed Aug 4, 2023

View reviewed changes

Address comments.

0d78eeb

starcke requested a review from a team as a code owner August 7, 2023 08:48

github-actions Bot added the C# label Aug 7, 2023

starcke added the no-change-note-required This PR does not need a change note label Aug 7, 2023

starcke added 2 commits August 8, 2023 15:02

Merge branch 'main' into starcke/automodel-pack

8d34ab6

Take filter pack into account.

3918e57

aeisenberg previously approved these changes Aug 14, 2023

View reviewed changes

Merge branch 'main' into starcke/automodel-pack

56871c7

starcke dismissed aeisenberg’s stale review via 56871c7 August 17, 2023 08:08

Update filter to point to right pack.

1b31c4d

Java: update model exclusions logic to cope with new automodel test l…

480e3bf

…ocation

starcke added 2 commits August 22, 2023 09:37

Update publish script to have right path.

3aeacf6

Released pack.

3b8b33a

aeisenberg previously approved these changes Aug 22, 2023

View reviewed changes

Merge branch 'main' into starcke/automodel-pack

361ae17

starcke dismissed aeisenberg’s stale review via 361ae17 August 30, 2023 07:26

charisk approved these changes Aug 30, 2023

View reviewed changes

starcke merged commit 44a83a7 into main Aug 30, 2023

starcke deleted the starcke/automodel-pack branch August 30, 2023 11:24

	WORKSPACE_ROOT="$AUTOMODEL_ROOT/../../../.."
	WORKSPACE_ROOT="$(realpath "$AUTOMODEL_ROOT/../../..")"

	AUTOMODEL_ROOT="$(dirname $0)"
	AUTOMODEL_ROOT="$(realpath "$(dirname $0)")"

Uh oh!

Conversation

starcke commented Aug 3, 2023

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

starcke commented Aug 3, 2023

Uh oh!

kaeluka commented Aug 3, 2023

Uh oh!

aeisenberg commented Aug 3, 2023

Uh oh!

aeisenberg Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeisenberg Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeisenberg commented Aug 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

starcke Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

starcke commented Aug 9, 2023

Uh oh!

kaeluka commented Aug 9, 2023

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

starcke commented Aug 17, 2023

Uh oh!

starcke commented Aug 17, 2023

Uh oh!

kaeluka commented Aug 17, 2023

Uh oh!

kaeluka commented Aug 17, 2023

Uh oh!

kaeluka commented Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

starcke commented Aug 18, 2023

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

aeisenberg Aug 3, 2023 •

edited

Loading

aeisenberg Aug 4, 2023 •

edited

Loading

starcke Aug 7, 2023 •

edited

Loading

kaeluka commented Aug 18, 2023 •

edited

Loading